-
Notifications
You must be signed in to change notification settings - Fork 330
Add new best practices for structured logging and accepting loggers #370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new best practices for structured logging and accepting loggers #370
Conversation
Sources/Logging/Docs.docc/BestPractices/002-StructuredLogging.md
Outdated
Show resolved
Hide resolved
|
|
||
| #### Avoid: Libraries creating their own loggers | ||
|
|
||
| Libraries might create their own loggers; however, this leads to two problems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth adding a caveat: libraries should default to creating a no-op logger if the user didn't pass one in, that makes it easier to ensure that the logger is always propagated through the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally didn't add this since I think this is a topic of contention right now. I personally think this is a bad pattern since I have seen many cases where the default parameter led to folks not passing in their logger and when debugging they didn't get log statements. I personally much rather prefer having a non-optional non-defaulted parameter since it requires users attention. However, I think there is design space here by leveraging structured concurrency more to avoid all these manual passings of loggers altogether.
c2de357 to
8f34b9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor grammar/wording tweaks and punctuation suggestions
Sources/Logging/Docs.docc/BestPractices/001-ChoosingLogLevels.md
Outdated
Show resolved
Hide resolved
Sources/Logging/Docs.docc/BestPractices/001-ChoosingLogLevels.md
Outdated
Show resolved
Hide resolved
Sources/Logging/Docs.docc/BestPractices/002-StructuredLogging.md
Outdated
Show resolved
Hide resolved
Sources/Logging/Docs.docc/BestPractices/002-StructuredLogging.md
Outdated
Show resolved
Hide resolved
Sources/Logging/Docs.docc/BestPractices/003-AcceptingLoggers.md
Outdated
Show resolved
Hide resolved
Sources/Logging/Docs.docc/BestPractices/003-AcceptingLoggers.md
Outdated
Show resolved
Hide resolved
Sources/Logging/Docs.docc/BestPractices/003-AcceptingLoggers.md
Outdated
Show resolved
Hide resolved
Sources/Logging/Docs.docc/BestPractices/003-AcceptingLoggers.md
Outdated
Show resolved
Hide resolved
Sources/Logging/Docs.docc/BestPractices/003-AcceptingLoggers.md
Outdated
Show resolved
Hide resolved
Sources/Logging/Docs.docc/BestPractices/003-AcceptingLoggers.md
Outdated
Show resolved
Hide resolved
8f34b9a to
c2b83ea
Compare
c2b83ea to
fba1c8b
Compare
|
The “For libraries” paragraph in I'm attaching a git patch fixing the issue: |
Please make a separate PR about this, this isn't PR about that document; let's keep the discussions and fixes separate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please make two PRs next time when adding unrelated documents though
|
|
||
| ### Metadata key conventions | ||
|
|
||
| Use hierarchical dot-notation for related fields: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah dots is a good recommendation I think... if some specific backend needs something else they can do what prometheus does in metrics for labels (nameAndLabelSanitizer) 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are those considered equivalent?
logger.debug(
"Database operation completed",
metadata: [
"db.operation": "SELECT",
"db.table": "users",
"db.duration": "\(duration)",
"db.rows": "\(rowCount)"
]
)
and
logger.debug(
"Database operation completed",
metadata: [
"db": [
"operation": "SELECT",
"table": "users",
"duration": "\(duration)",
"rows": "\(rowCount)"
]
]
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No they are not. They result in different metadata being generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second one also requires another dictionary allocation
| When libraries accept loggers as method parameters, they enable automatic | ||
| propagation of contextual metadata attached to the logger instance. This is | ||
| especially important for distributed systems where correlation IDs and tracing | ||
| information must flow through the entire request processing pipeline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best practices listed are OK, however it somewhat pretty weird to call out tracing as motivation and not mention the way tracing actually integrates with logging -- which isn't this pattern (regardless of opinions on status quo).
I'd just drop "distributed tracing" from the motivation entirely if the examples are not going to address it at all, causes some confusion as it's not showing off how currently tracing actually integrates with loggers today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dropped tracing and just kept the correlation IDs part.
Ok, I’ll create a separate PR for that. Honestly, I already had a draft PR prepared for my change/suggestion, but later I found out that this PR already exists, which among other things also modifies the |
|
@DominikPalo I included your change here since I already have some changes in that file. |
fba1c8b to
f406145
Compare
Sources/Logging/Docs.docc/BestPractices/001-ChoosingLogLevels.md
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,100 @@ | |||
| # 003: Accepting loggers in libraries | |||
|
|
|||
| Accept loggers through method parameters to ensure proper metadata propagation. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you consider splitting this one into a separate PR to allow us to discuss more, without blocking the other changes? I have more thoughts here.
|
@swift-ci please test |
f406145 to
4c0d1af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still lgtm :-)
4c0d1af to
8bf585c
Compare
|
The Xcode CI is expected to fail since the runner currently doesn't have the right iOS simulator installed |
No description provided.